-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initialize Internal APIs for components #1304
base: feature-operator-refactor
Are you sure you want to change the base?
Initialize Internal APIs for components #1304
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
}), builder.WithPredicates(dashboardPredicates)). | ||
Watches(&appsv1.ReplicaSet{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { | ||
return r.watchDashboardResources(ctx, a) | ||
}), builder.WithPredicates(dashboardPredicates)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably not needed
Watches(&corev1.PersistentVolumeClaim{}, handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { | ||
return r.watchDashboardResources(ctx, a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probabaly only need to be added in workbench SetupWithManager()
we can refine them one component by component
DeleteFunc: func(e event.DeleteEvent) bool { | ||
labelList := e.Object.GetLabels() | ||
if value, exist := labelList[labels.ODH.Component(ComponentNameUpstream)]; exist && value == "true" { | ||
return true | ||
} | ||
return false | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want apply label on Dashboard CR as well just like all the other resources we are doing now?
(cherry picked from commit 086fe25)
Co-authored-by: Wen Zhou <[email protected]>
Make it possible to compile operator without webhook enabled (with -tags nowebhook). Create a stub webhook.Init() function for that. Add run-nowebhook target to run webhook locally. It requires `make install` to be executed on the cluster beforehand. Since it repeats `make run`, move the command to a variable. Also use variable RUN_ARGS for operator arguments. It makes it possible to override them from the command line. In VSCode it is possible to debug it with the following example launch.json configuration: ``` { "name": "Debug Operator No Webhook", "type": "go", "request": "launch", "mode": "debug", "program": "main.go", "buildFlags": [ "-tags", "nowebhook" ], "env": { "OPERATOR_NAMESPACE": "opendatahub-operator-system", "DEFAULT_MANIFESTS_PATH": "./opt/manifests" }, "args": [ "--log-mode=devel" ], "cwd": "${workspaceFolder}", } ``` Signed-off-by: Yauheni Kaliuta <[email protected]>
(cherry picked from commit 4d5874d)
…1:Dashboard (cherry picked from commit f3738fd)
406f1eb
to
ecd060e
Compare
ecd060e
to
59edbbf
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature-operator-refactor #1304 +/- ##
============================================================
Coverage ? 19.98%
============================================================
Files ? 32
Lines ? 3438
Branches ? 0
============================================================
Hits ? 687
Misses ? 2684
Partials ? 67 ☔ View full report in Codecov by Sentry. |
LGTM |
/hold |
The option is deprecated by opendatahub-io#1304 Signed-off-by: Yauheni Kaliuta <[email protected]>
} | ||
} | ||
|
||
if rr.DSCI.Spec.DevFlags != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove that, deprecated by coming #1307
components.Component `json:""` | ||
} | ||
|
||
func (d *Dashboard) Init(ctx context.Context, platform cluster.Platform) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is should not be reverted, it is sort of regression.
return nil | ||
} | ||
|
||
func CreateDashboardInstance(dsc *dscv1.DataScienceCluster) *componentsv1.Dashboard { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just allocates a structure with some predefined fields. Can it be named less dramatic :) ? NewDashboard() or something?
4b40c43
to
af2c1b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gotten all of the way through this yet, but leaving some minor comments from this pass.
Again, I'm very rusty when it comes to Go and operators currently, so please excuse any comments that don't make sense! 🙂
controller: true | ||
domain: opendatahub.io | ||
group: components | ||
kind: Kserve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind: Kserve | |
kind: KServe |
I guess this might be slightly preferred? I don't know what the implications of one over the other are though.
@@ -25,13 +25,14 @@ import ( | |||
ctrl "sigs.k8s.io/controller-runtime" | |||
"sigs.k8s.io/controller-runtime/pkg/client" | |||
|
|||
"github.com/opendatahub-io/opendatahub-operator/v2/apis/components" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is also imported on L29 as componentsold, are both intentional?
Description
This PR includes following changes. Some of the items are yet to be completed:
How Has This Been Tested?
CatalogSource: quay.io/vhire/opendatahub-operator-catalog:v2.19.4
Dashboard
CR is createdScreenshot or short clip
Merge criteria